fix(mcp): use atomic.Bool for Server.readonly to prevent data races#3759
Conversation
knative-prow-robot
added
the
needs-rebase
knative-prow
Bot
added
size/S
|
Hi @Elvand-Lie. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Elvand-Lie
force-pushed
the
fix/mcp-readonly-data-race
branch
from
1a97cc3 to
80c4e87
Compare
knative-prow-robot
removed
the
needs-rebase
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3759 +/- ##
==========================================
+ Coverage 57.06% 57.10% +0.03%
==========================================
Files 181 181
Lines 21145 21145
==========================================
+ Hits 12067 12074 +7
+ Misses 7855 7849 -6
+ Partials 1223 1222 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Elvand-Lie
force-pushed
the
fix/mcp-readonly-data-race
branch
from
80c4e87 to
09b5041
Compare
knative-prow
Bot
added
size/XL
The Server.readonly field is a plain bool with no synchronization. While the current stdio transport is single-client, the MCP SDK may dispatch tool calls on separate goroutines internally. Additionally, Start() writes to readonly while handler goroutines may concurrently read it, which is a data race under the Go memory model. Replace bool with sync/atomic.Bool for lock-free thread safety. All reads use Load() and all writes use Store(). Also add test coverage for the readonly rejection path in both the deploy and delete handlers. Signed-off-by: Elvand Lie <elvandlie@gmail.com> Signed-off-by: elvandlie@gmail.com <elvandlie@gmail.com>
Elvand-Lie
force-pushed
the
fix/mcp-readonly-data-race
branch
from
09b5041 to
c47eb3b
Compare
knative-prow
Bot
added
size/M
|
@Elvand-Lie: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Thanks! This is not immediately apparent nuance. Would be nice to add this as a comment inline above the synchronized member for future reference. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Elvand-Lie, lkingland The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow
Bot
added
the
approved
|
/retest |
3 participants
Summary
Fixes #3756
The
Server.readonlyfield is a plainboolwith no synchronization. While the current stdio transport is single-client, the MCP SDK may dispatch tool calls on separate goroutines internally. Additionally,Start()writes toreadonlywhile handler goroutines may concurrently read it, which is a data race under the Go memory model.Changes
pkg/mcp/mcp.go: Replaceboolwithsync/atomic.Bool. All reads use.Load(), all writes use.Store().pkg/mcp/tools_deploy.go: Update readonly check to use.Load().pkg/mcp/tools_delete.go: Update readonly check to use.Load().tools_deploy_test.goandtools_delete_test.goto use.Store().Testing